-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ovsx-client: introduce async ovsx-client
provider
#10327
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that the issue exists on master
and is addressed by the changes nicely 👍
I have a suggestion before merging.
packages/vsx-registry/src/browser/vsx-registry-frontend-module.ts
Outdated
Show resolved
Hide resolved
9502b0d
to
8bdaa21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The commit fixes an issue where previously the backend would never properly read the cli arguments in time to create the ovsx-client. This meant that while the api-version was respected when searching in the view, it was not when installing. The change removes the use of `OVSXAsyncClient` and instead uses an `async` provider of `OVSXClient` which will probably be set with the correct api version and url when ready. Signed-off-by: vince-fugnitto <[email protected]>
Signed-off-by: vince-fugnitto <[email protected]>
Signed-off-by: vince-fugnitto <[email protected]>
7132a82
to
214f5ee
Compare
@msujew @colin-grant-work the pull-request was further update following some feedback and ideas from @paul-marechal where we now properly wait for the environment and cli arguments to be ready before we set the api-level which is then used when creating the client. The new approach should now properly respect the api-level and no longer rely on timing like it was previously done. The following should hold true:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know some of it is my own code, but along with @vince-fugnitto's original work it now looks good to me!
We no longer rely on timing and rather explicitly wait for the right data to be set (waiting for the cli arguments to be parsed, fallback to the env if not specified, and finally using a hardcoded default if not in the env.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for awaiting proper timing. 👍
What it does
Fixes: #10326.
The pull-request fixes the issue #10326 which was caused by the way the
OVSXClient
was initialized in the backend. Previously, the client was created statically at a point in which the backend was not able to read the command line arguments in an asynchronous manner. Rather than duplicating the work we do in the frontend, and the use of the "hackish"OVSXAsyncClient
the following was used:The main change is to introduce an
OVSXClientProvider
which is in charge of providing a promise to anOVSXClient
when ready (meaning when we finally read the proper api version and registry url).How to test
Try the steps highlighted in #10326:
--vscode-api-version=1.40.0
as an argument (sets the api to1.40.0
)v11.2.1
and remains atv10.2.3
Review checklist
Reminder for reviewers
Signed-off-by: vince-fugnitto [email protected]